-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
JuliaSyntax parser-based REPL completions overhaul #57767
base: master
Are you sure you want to change the base?
Conversation
Adds another permitted return type for complete_line, where the second element of the tuple is a Region (a Pair{Int, Int}) describing the region of text to be replaced. This is useful for making completions work consistently when the closing delimiter may or may not be present: the cursor can be made to "jump" out of the delimiters regardless of whether it is there already. "exam| =TAB=> "example.jl"| "exam|" =TAB=> "example.jl"|
This commit replaces the heuristic parsing done by REPLCompletions.completions with a new approach that parses the entire input buffer once with JuliaSyntax. In addition to fixing bugs, the more precise parsing should allow for new features in the future. Some features now work in more situations "for free", like dictionary key completion (the expression evaluated to find the keys is now more precise) and method suggestions (arguments beyond the cursor can be used to narrow the list). The tests have been updated to reflect slightly differing behaviour for string and Cmd-string completion: the new code returns a character range encompassing the entire string when completing paths (not observable by the user), and the behaviour of '~'-expansion has be tweaked to be consistent across all places where paths can be completed. Some escaping issues have also been fixed. Fixes: JuliaLang#55420, JuliaLang#55518, JuliaLang#55520, JuliaLang#55842, JuliaLang#56389, JuliaLang#57611
This sounds great! I think we should backport it to at least 1.12 given it fixes so many issues. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bravo. Truly excellent quality-of-life PR 👏🏻 |
Best part is that this PR has a negative diff, depsite the fact it added many tests. |
…JuliaLang#57307, JuliaLang#57624 Also fix test failing for silly reason
Since we parse the entire input now, we need to avoid triggering method completion when the cursor is on the function name of a valid call.
stdlib/REPL/src/REPLCompletions.jl
Outdated
|
||
# Expand '~' if the user hits TAB after exhausting completions (either | ||
# because we have found an existing file, or there is no such file). | ||
full_path = ispath(path) || isempty(paths) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found two cases that can cause ispath
to error, so I think we should make a safe_ispath
-
ArgumentError: embedded NULs are not allowed in C strings: "invalid\0"
forlet name = \"valid.txt\", name⁺ = \"invalid\\0.txt\"
-
IOError name too long (ENAMETOOLONG) for strings that are too long.
Something like this? Or just never throw and just return false?
function safe_ispath(path::AbstractString)
try
return ispath(path)
catch err
if err isa Base.IOError # i.e. name too long (ENAMETOOLONG)
return false
elseif err isa Base.ArgumentError && occursin("embedded NULs", err.msg)
return false
else
rethrow()
end
end
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe call it ispath_noerror
or make it a keyword to ispath
? The implication that ispath
is unsafe or that this is "safer" feels unwarranted—it's just that there's situations where you don't want any errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll avoid modifying Base for the time being because we already convert some access(2)
return values to false
. It feels a little hacky to keep adding exceptions here.
Lines 376 to 379 in bc98abc
r = ccall(:jl_fs_access, Cint, (Cstring, Cint), path, F_OK) | |
if !(r in (0, Base.UV_ENOENT, Base.UV_ENOTDIR, Base.UV_EINVAL)) | |
uv_error(string("ispath(", repr(path), ")"), r) | |
end |
Of the errors that could be returned for F_OK
that we might want to handle, it looks like we're missing EACCES
, EIO
, ELOOP
, and ENAMETOOLONG
.
Co-authored-by: Ian Butterworth <[email protected]>
Co-authored-by: Ian Butterworth <[email protected]>
Co-authored-by: Mosè Giordano <[email protected]>
|
I am going to disable the shell completion tests until the shell mode can parse Windows paths...
|
Also cleans up do_cmd_escape, so that it can use different escaping syntax from the shell mode (which we may want to make similar to cmd.exe on Windows).
The shell mode has no problem with Windows paths which is why the REPL has tests for it. It just sounds like you lost a call to |
for name in get_loading_candidates(pkgstarts, project_file) | ||
push!(suggestions, PackageCompletion(name)) | ||
function complete_loading_candidates!(suggestions::Vector{Completion}, s::String) | ||
for name in ("Core", "Base") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite know the context of this, but should this be something like:
for name in ("Core", "Base") | |
for name in (r"Core\>", r"Base\>") |
Since we have various modules like CoreLogging
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have an example where it misbehaves? This should only fire when we're completing the first component of an import path with no leading dots.
end | ||
|
||
function shell_completions(string, pos, hint::Bool=false) | ||
function shell_completions(string, pos, hint::Bool=false; cmd_escape::Bool=false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we always use the shell parser, why would we ever want this set true? I believe in the past, the code decided between whether it was completing one argument or several arguments, with escaping thus either wanting to include or exclude spaces. And thus choosing to fail if the input contained space-separate arguments rather than handling them, so I don't know why that would ever be useful.
# escape_raw_string with delim='`' and ignoring the rule for the ending \ | ||
return replace(s, r"(\\+)`" => s"\1\\`") | ||
function do_cmd_escape(s) | ||
return Base.shell_escape_posixly(Base.escape_raw_string(s, '`')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like nonsense: I can't think of any case where we'd expect it'd be useful for the the REPL to pass the string unmodified to a posix shell for use as an argument in a julia script, which is what this transform order implements.
The previous transform took a string text
that was already in a valid posix-shell form (e.g. for raw input to shell_parse for shell> text
mode) and corrects it for the julia parser (e.g. for :(`text`)
). Usually our shell_escape_posixly
attempts to add quotes to avoid this edge case complexity, but if we have chosen to preserve the user-typed syntax then this is needed
julia> a = `\\ b\\\\`
`'\' 'b\'`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I intended cmd_escape
to make it possible to complete shell commands inside Cmd-strings, but mixed up the order (Base.escape_raw_string(Base.shell_escape_posixly(s), '`')
does what I want I think):
julia> println(R.do_cmd_escape("file ` 1"))
'file \` 1'
julia> @macroexpand `'file \` 1'`
:(Base.cmd_gen((("file ` 1",),)))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Completion with shell_escape_posixly
is still unwieldy because you get partial completions that insert only the opening quote, and can't complete any further.
Co-authored-by: Jameson Nash <[email protected]>
Overview
As we add REPL features, bugs related to the ad-hoc parsing done by
REPLCompletions.completions
have crept in. This pull request replaces most ofthe manual parsing (regex,
find_start_brace
) with a new approach that parsesthe entire input buffer once, before and after the cursor, using JuliaSyntax.
We then query the parsed syntax tree to determine the kind of completion
to be done.
Changes
New, JuliaSyntax-based completions mechanism.
The
complete_line
interface now has the option of replacing arbitraryregions of text in the input buffer by returning a
Region
(Pair{Int, Int}
for consistency with the convention in LineEdit, and
pos
being a 0-basedbyte offset).
This is used to improve the handling of auto-inserted closing. Leaving this unchanged for now.delimiters.
Fixes parsing-related bugs:
ModuleName.function
call is broken #55420@time
calls involving another macro #55429ModuleName.@macro_name
is broken #55518@benchmark A .= A setup=(A=
is broken #55520begin-end
block #56389v = Module.Submodule.<TAB>
is broken #57611Fixes some bugs that exist on 28d3bd5 that were found by fuzzing:
x \"
+TAB
throws aFieldError
exceptionThe duplicate code for path completion in strings,
Cmd
-strings, and theshell has been removed, causing paths to complete the same way for all three.
Now,
~
is expanded in two situations:If
foo
exists, or iffoo
does not exist but there are nopossible completions:
If the current path ends with a
/
and you hit TAB again:Future work
Method completions could be changed to look for methods with exactly the given
number of arguments if the closing
)
is present, and search for signatureswith the right prefix otherwise.
by putting
::T
in place of arguments).Other REPL features could benefit from JuliaSyntax, so it might be worth
sharing the parse tree between completions and other features:
C-M-f
/C-M-b
/C-M-u
, etc.It would be nice if hints worked even when the cursor is between text.
CursorNode
is a slightly tweaked copy ofSyntaxNode
from JuliaSyntax thattracks the parent node but includes all trivia. It is used with
seek_pos
,which navigates to the innermost node at a given position so we can examine
nearby nodes and the parent. This could probably duplicate less code from
JuliaSyntax.